Created PAT visual for managing - #338
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds verified-user PAT management to Settings, with token listing, creation, one-time reveal/copy, and revocation in the web dashboard. PAT API handlers now reject missing or unverified users before processing token actions. ChangesPAT Management Flow
Sequence Diagram(s)sequenceDiagram
participant User
participant Settings
participant PATManager
participant dashboard-api
participant BrowserClipboard
User->>Settings: open Settings
Settings->>PATManager: render PAT Visual for verified user
PATManager->>dashboard-api: GET /api/user/pats
dashboard-api-->>PATManager: token list
User->>PATManager: submit new token label and TTL
PATManager->>dashboard-api: POST /api/user/pats
dashboard-api-->>PATManager: created token + rawToken
PATManager-->>User: show one-time reveal modal
User->>PATManager: copy rawToken
PATManager->>BrowserClipboard: navigator.clipboard.writeText(rawToken)
User->>PATManager: confirm revoke
PATManager->>dashboard-api: DELETE /api/user/pats/:id
dashboard-api-->>PATManager: revoke result
PATManager-->>User: remove token from table
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/dashboard-api/src/controllers/pat.controller.js`:
- Around line 83-89: In pat.controller.js, the revocation flow in the controller
that clears `cli:pat:cache:${patToRevoke.tokenHash}` is fire-and-forget, so the
success response can be sent before cache invalidation completes. Update the PAT
revoke handler to `await` the `redis.del()` call, and if it fails or Redis is
unreachable, stop the request and return a 503 via `AppError` instead of
confirming revocation. Keep the database delete and cache clear coordinated in
the same revoke path so the response is only sent after cache invalidation
succeeds.
In `@apps/web-dashboard/src/components/PATManager.jsx`:
- Around line 212-215: The custom token modal overlays in PATManager are missing
dialog semantics for assistive tech. Update the fixed overlay containers used by
the create and other token modals (including the sections controlled by
showCreateModal and the matching modal block later in the component) to expose
role="dialog" and aria-modal, and bind them to a visible heading via an
accessible label such as aria-labelledby on the modal title element. Ensure the
existing heading text like “Generate New Token” is used so screen readers
announce the modal context correctly.
- Around line 180-200: The revoke action in PATManager is not accessible because
the action column header is empty and the button is effectively icon-only.
Update the table header and the revoke control in PATManager so assistive
technologies have an explicit name to announce, using the existing revoke button
handler and revoke state identifiers (such as setRevokeId and the revoke button
markup) to add a clear accessible label.
- Around line 136-142: The copyToClipboard callback in PATManager.jsx sets the
copied state before the clipboard write completes, which can show a false
success. Update copyToClipboard to await navigator.clipboard.writeText(text)
inside try/catch, and only call setCopied(true) after the write succeeds. If the
write fails, do not flip the copied state; keep the existing timer cleanup logic
in place and locate the fix in the copyToClipboard useCallback.
In `@apps/web-dashboard/src/pages/Settings.jsx`:
- Around line 197-198: The PAT access control is only enforced in the UI via
Settings.jsx, so unverified users can still hit the backend PAT APIs. Update the
server-side auth flow by adding an isVerified authorization check in the PAT
route pipeline or a dedicated middleware used by pat.controller methods
createPAT, listPATs, and revokePAT, and ensure authMiddleware propagates
req.user.isVerified so these endpoints reject unverified users consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7f1cf37-88a6-4710-86f9-082e8b416d1a
📒 Files selected for processing (3)
apps/dashboard-api/src/controllers/pat.controller.jsapps/web-dashboard/src/components/PATManager.jsxapps/web-dashboard/src/pages/Settings.jsx
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/dashboard-api/src/controllers/pat.controller.js (1)
97-100: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDon't report PAT revocation success when cache invalidation fails.
await redis.del(...)is now synchronous, but this catch still logs and falls through to a 200. If Redis fails here, the DB row is gone while the cached PAT can remain usable until expiry/eviction. Return anAppError(for example 503) instead of confirming revocation.Proposed fix
try { await redis.del(`cli:pat:cache:${patToRevoke.tokenHash}`); } catch (redisErr) { console.error("Failed to clear PAT from Redis cache:", redisErr); + return next(new AppError(503, "Unable to revoke token right now")); }As per coding guidelines, “Use AppError class for errors.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/dashboard-api/src/controllers/pat.controller.js` around lines 97 - 100, The PAT revocation flow in pat.controller.js currently logs Redis cache invalidation failures and still returns success, which can report revocation even when the cached PAT remains usable. Update the revocation logic around the redis.del call in the controller method that handles PAT revocation to throw an AppError instead of falling through when cache clearing fails, and make sure the failure response uses an appropriate status like 503. Preserve the existing success path only after both the DB deletion and Redis invalidation complete successfully.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/dashboard-api/src/controllers/pat.controller.js`:
- Around line 5-8: The new access-control branches in pat.controller.js are
returning AppError through next(), which produces the wrong response envelope
for these endpoints. Update the guards in the PAT controller methods that
currently short-circuit with AppError so they return the standard API shape
directly with success, data, and message fields instead of relying on the global
error handler. Keep the checks in the same controller functions that guard
req.user and req.user.isVerified, and apply the same fix to all three new 403
exits referenced in this diff.
---
Duplicate comments:
In `@apps/dashboard-api/src/controllers/pat.controller.js`:
- Around line 97-100: The PAT revocation flow in pat.controller.js currently
logs Redis cache invalidation failures and still returns success, which can
report revocation even when the cached PAT remains usable. Update the revocation
logic around the redis.del call in the controller method that handles PAT
revocation to throw an AppError instead of falling through when cache clearing
fails, and make sure the failure response uses an appropriate status like 503.
Preserve the existing success path only after both the DB deletion and Redis
invalidation complete successfully.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d8ef952-9d72-42cd-8a23-b4429457d1d5
📒 Files selected for processing (2)
apps/dashboard-api/src/controllers/pat.controller.jsapps/web-dashboard/src/components/PATManager.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web-dashboard/src/components/PATManager.jsx
| if (!req.user || !req.user.isVerified) { | ||
| return next(new AppError(403, 'Forbidden: You must verify your account to access Personal Access Tokens.')); | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The new 403 branches return the wrong API envelope.
These guards call next(new AppError(...)), and the current global handler serializes that as { success: false, error, message }, not the required { success, data, message }. Since this PR adds three new error exits, it also adds three new contract-breaking responses for these endpoints.
As per coding guidelines, “All API endpoints must return response format: { success: bool, data: {}, message: "" }.”
Also applies to: 55-58, 83-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/dashboard-api/src/controllers/pat.controller.js` around lines 5 - 8, The
new access-control branches in pat.controller.js are returning AppError through
next(), which produces the wrong response envelope for these endpoints. Update
the guards in the PAT controller methods that currently short-circuit with
AppError so they return the standard API shape directly with success, data, and
message fields instead of relying on the global error handler. Keep the checks
in the same controller functions that guard req.user and req.user.isVerified,
and apply the same fix to all three new 403 exits referenced in this diff.
Source: Coding guidelines
There was a problem hiding this comment.
yes please follow the response type style
@Ayush4958
There was a problem hiding this comment.
@yash-pouranik sounds good — the three verified-user guard exits in apps/dashboard-api/src/controllers/pat.controller.js should return the standard API envelope directly, i.e. { success, data, message }, instead of delegating the 403 through next(new AppError(...)).
Once that’s updated in createPAT, listPATs, and revokePAT, this concern should be resolved.
|
@yash-pouranik |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| if (!req.user || !req.user.isVerified) { | ||
| return next(new AppError(403, 'Forbidden: You must verify your account to access Personal Access Tokens.')); | ||
| } | ||
|
|
There was a problem hiding this comment.
yes please follow the response type style
@Ayush4958
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@yash-pouranik |
|
I literraly forgot |
|
@coderabbitai new files added in this PR and location |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Get PR metadata to identify the base branch for comparison
base_ref=$(gh pr view 338 --json baseRefName --jq '.baseRefName')
echo "BASE_REF=$base_ref"
# Ensure the base branch ref is available locally
git fetch origin "$base_ref" --depth=1 >/dev/null 2>&1 || true
echo
echo "Added files in PR `#338`:"
git diff --name-status "origin/$base_ref...HEAD" | awk '$1=="A" {print $2}'Length of output: 800 🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 338 --repo geturbackend/urBackend --json files \
--jq '.files[] | select(.additions > 0 and .deletions == 0) | [.path] | `@tsv`'Length of output: 400 🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 338 --repo geturbackend/urBackend --json files \
--jq '.files[] | [.path, .status] | `@tsv`'Length of output: 420 🏁 Script executed: #!/bin/bash
set -euo pipefail
gh api repos/geturbackend/urBackend/pulls/338/files --paginate \
--jq '.[] | [.filename, .status] | `@tsv`'Length of output: 445
For clarity, these other changed files are modified, not newly added:
|
3 participants
🚀 Pull Request Description
I had created web visual for managing the PAT for user
Screen Recording
Screen.Recording.2026-06-26.151926.mp4
🛠️ Type of Change
🧪 Testing & Validation
Backend Verification:
npm testin thebackend/directory and all tests passed.Frontend Verification:
npm run lintin thefrontend/directory.✅ Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Security
Bug Fixes